Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: AddRingBuf() using ring_buffer__add #430

Merged
merged 1 commit into from
Jun 26, 2024
Merged

feat: AddRingBuf() using ring_buffer__add #430

merged 1 commit into from
Jun 26, 2024

Conversation

mighty1231
Copy link
Contributor

@mighty1231 mighty1231 commented May 20, 2024 •
edited
Loading

libbpf provides ring_buffer__add to handle multiple ringbuf event callbacks

  • single RingBuffer object now handles multiple slots
  • implements AddRingBuf() for ring_buffer__add wrapper
  • updates ringbuffers selftest to show how to handle it

Reference

@CLAassistant
Copy link

CLAassistant commented May 20, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@mighty1231 mighty1231 force-pushed the feat/add-ring-buf branch 2 times, most recently from 26b1112 to f4f8407 Compare May 20, 2024 06:19
@geyslan
Copy link
Member

geyslan commented May 21, 2024

@mighty1231 tks for this PR. There's a formatting issue https://github.com/aquasecurity/libbpfgo/pull/430/checks. As it is solved I'll stash it for later review. Tks again.

@mighty1231
Copy link
Contributor Author

mighty1231 commented May 22, 2024 •
edited
Loading

@geyslan I ran make fmt-fix. Now make fmt-check returns no error

EDIT: with new amended commit, make lint-check also returns no error

@mighty1231 mighty1231 force-pushed the feat/add-ring-buf branch 2 times, most recently from 14e07f1 to 6b6b706 Compare May 27, 2024 00:43
@mighty1231
Copy link
Contributor Author

mighty1231 commented May 28, 2024 •
edited
Loading

Failed test map-getmapsbyname seems not related to my PR, because it seems to have no ringbuffer-related code, and it runs before the ringbuffers test.

@geyslan
Copy link
Member

geyslan commented May 28, 2024

@mighty1231, I suspect that self-test/iter sometimes fails due to ping usage and internal network malfunction. Well, it seems like a flaky test.

@geyslan geyslan mentioned this pull request May 30, 2024
Open
@geyslan geyslan self-requested a review June 6, 2024 20:35
@geyslan geyslan added the feature New feature or request label Jun 6, 2024
geyslan
geyslan requested changes Jun 6, 2024
View reviewed changes
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mighty1231 I tested it and it works nice. That's awesome.

I would just ask you to make the selftest more robust, since you have touched the logic of the slot (index) inside RingBuffer that's used by rwArray which .

Would you mind to improve the the respective selftest to submit a greater volume of data through a greater number of slots (ringbuffers) not reusing the same data holder? I mean, try using more than one program to submit the data.

libbpfgo.c Outdated
Comment on lines 66 to 70
fprintf(stderr, "Failed to add ring buffer: %s\n", strerror(-ret));
return ret;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the same pattern of cgo_init_ring_buf regarding saving and restoring errno.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it.

@mighty1231
feat: AddRingBuf() using ring_buffer__add
c0b5034 
libbpf provides ring_buffer__add to handle multiple ringbuf event
callbacks

- single RingBuffer object now handles multiple slots
- implements AddRingBuf() for ring_buffer__add wrapper
- updates ringbuffers selftest to show how to handle it

Reference
- https://github.com/torvalds/linux/blob/eb6a9339efeb6f3d2b5c86fdf2382cdc293eca2c/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
- https://github.com/torvalds/linux/blob/eb6a9339efeb6f3d2b5c86fdf2382cdc293eca2c/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c#41
@mighty1231
Copy link
Contributor Author

mighty1231 commented Jun 10, 2024 •
edited
Loading

@mighty1231 I tested it and it works nice. That's awesome.

I would just ask you to make the selftest more robust, since you have touched the logic of the slot (index) inside RingBuffer that's used by rwArray which .

Would you mind to improve the the respective selftest to submit a greater volume of data through a greater number of slots (ringbuffers) not reusing the same data holder? I mean, try using more than one program to submit the data.

I am confused. Which of the following did you mean?

  1. More tests to check thread safety of Module *AddRingBuf(), RingBuffer *Stop(), and RingBuffer *Close(), which modifies RingBuffer.slots
  2. More tests with two or more struct *RingBuffer in golang
  3. More tests wtih two or more NewModuleFromFile("main.bpf.o") call
  4. More tests wtih two or more probes - other than kprobe__sys_mmap from selftest/ringbuffers/main.bpf.c
  5. Make similar to ringbuf_multi test from linux repository - ringbuf_multi.c and test_ringbuf_multi.c

1 may be resolved with notifying the limitation for users. If it doesn't seem right, I would appreciate any feedback you may have.
2 is the motivation of this PR. I have got completely lost to use more than one RingBuffer, so I decided to use ring_buffer__add.
3, 4 - it's difficult to guess what can be improved for it
5 seems interesting. if you mean for it, I will take it

@geyslan
Copy link
Member

geyslan commented Jun 10, 2024 

5. Make similar to ringbuf_multi test from linux repository

It would be awesome to have a huge amount of tests like ringbuf_multi. Anyway, I think it would take longer.

I meant:

  • increasing numberOfEvent*Received* to a considerable amount.
  • split the recvLoop for select into the amount of ringbuf slots, raising one goroutine for each.

@mighty1231
Copy link
Contributor Author

mighty1231 commented Jun 11, 2024 •
edited
Loading

I don't know how to split the for-select loop, since I don't know golang much. And I didn't understand what is improved with new test.

@geyslan
Copy link
Member

geyslan commented Jun 13, 2024

@NDStrahilevitz do you mind reviewing this?

@geyslan
Copy link
Member

geyslan commented Jun 26, 2024

@mighty1231 I'm merging this. Thanks again for your contrib.

@geyslan geyslan merged commit 560e67b into aquasecurity:main Jun 26, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geyslan geyslan geyslan approved these changes

Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mighty1231 @CLAassistant @geyslan